Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Visit ImplItem in dead_code lint #59486

Merged
merged 3 commits into from
Mar 29, 2019
Merged

Visit ImplItem in dead_code lint #59486

merged 3 commits into from
Mar 29, 2019

Conversation

varkor
Copy link
Member

@varkor varkor commented Mar 28, 2019

Fixes #47131.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2019
@Centril
Copy link
Contributor

Centril commented Mar 28, 2019

@bors rollup

Centril
Centril reviewed Mar 28, 2019
View reviewed changes
src/librustc/middle/dead.rs
@@ -172,6 +172,7 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {
| hir::ItemKind::Ty(..)
| hir::ItemKind::Static(..)
| hir::ItemKind::Existential(..)
| hir::ItemKind::Impl(..)
| hir::ItemKind::Const(..) => {
intravisit::walk_item(self, &item);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove _ => () and replace with "exhaustive" match to avoid future problems?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure why this is not

_ => {
    intravisit::walk_item(self, &item);
}

If that doesn't break anything, then walk_item can be moved out of the match.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2019
@varkor
Copy link
Member Author

varkor commented Mar 28, 2019

I've made the dead_code lint walk all items by default. I had to exclude ItemKind::ForeignMod, which caused a false negative and explicitly ignore a few Defs, which were now visited, but otherwise there weren't any problems. This should lower the chance an item is accidentally missed now.

@varkor varkor mentioned this pull request Mar 29, 2019
Open
@sanxiyn
Copy link
Member

sanxiyn commented Mar 29, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Mar 29, 2019

📌 Commit 8cdfad9 has been approved by sanxiyn

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 29, 2019
@petrochenkov petrochenkov assigned sanxiyn and unassigned petrochenkov Mar 29, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 29, 2019
@Centril
Rollup merge of rust-lang#59486 - varkor:dead-code-impl, r=sanxiyn
4aacc49 
Visit `ImplItem` in `dead_code` lint

Fixes rust-lang#47131.
@Centril Centril mentioned this pull request Mar 29, 2019
Merged
bors added a commit that referenced this pull request Mar 29, 2019
@bors
Auto merge of #59522 - Centril:rollup, r=Centril
d920dfc 
Rollup of 9 pull requests

Successful merges:

 - #59366 (Update books)
 - #59436 (Update jemalloc-sys to version 0.3.0)
 - #59454 (Update rustfmt to 1.2.0)
 - #59462 (Fix error in Rust 2018 + no_core environment)
 - #59467 (Better diagnostic for binary operation on BoxedValues)
 - #59473 (Do not emit incorrect borrow suggestion involving macros and fix overlapping multiline spans)
 - #59480 (Update stdsimd)
 - #59486 (Visit `ImplItem` in `dead_code` lint)
 - #59510 (Rename `type_parameters` to `generics` and so on)

Failed merges:

 - #59516 (Update cargo)

r? @ghost
bors added a commit that referenced this pull request Mar 29, 2019
@bors
Auto merge of #59522 - Centril:rollup, r=Centril
e782d79 
Rollup of 9 pull requests

Successful merges:

 - #59366 (Update books)
 - #59436 (Update jemalloc-sys to version 0.3.0)
 - #59454 (Update rustfmt to 1.2.0)
 - #59462 (Fix error in Rust 2018 + no_core environment)
 - #59467 (Better diagnostic for binary operation on BoxedValues)
 - #59473 (Do not emit incorrect borrow suggestion involving macros and fix overlapping multiline spans)
 - #59480 (Update stdsimd)
 - #59486 (Visit `ImplItem` in `dead_code` lint)
 - #59510 (Rename `type_parameters` to `generics` and so on)

Failed merges:

 - #59516 (Update cargo)

r? @ghost
@bors
Copy link
Contributor

bors commented Mar 29, 2019

☔ The latest upstream changes (presumably #59522) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 29, 2019
@bors bors merged commit 8cdfad9 into rust-lang:master Mar 29, 2019
@varkor varkor deleted the dead-code-impl branch March 29, 2019 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@Centril Centril Centril left review comments

Assignees

@sanxiyn sanxiyn

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@varkor @rust-highfive @Centril @sanxiyn @bors @petrochenkov